feat(resolver): resolve calls through Object.defineProperty / defineProperties / create#1328
Conversation
…r-typed resolution (JS/TS) Closes #1306 Extends `handlePropWriteTypeMap` to seed the points-to type map from `this.prop = new ClassName(...)` assignments, enabling resolution of `this.prop.method()` calls through the existing receiver-typed path. Before: `this.logger.error/info/warn()` in UserService were unresolvable because only `obj.prop = identifier` writes were tracked (Phase 8.3d). After: `this.logger = new Logger(...)` seeds `typeMap['this.logger'] = Logger` with confidence 1.0 (same as `const x = new Ctor()` in variable declarators). The existing `resolveByMethodOrGlobal` and native `edge_builder.rs` fallback (`or_else(|| type_map.get(receiver))`) then pick up `this.logger` and resolve `this.logger.error()` → `Logger.error`. Impact: JS fixture receiver-typed recall 2/5 → 5/5 (40% → 100%). Ratchet JS benchmark gate: precision 0.85→1.0, recall 0.5→0.9.
…M path (#1314) When a class method calls this.method() or super.method(), the WASM/JS resolution path failed to find the target because methods are stored as qualified names (e.g. Logger._write) but lookups used the unqualified name (_write). The native Rust engine resolved this implicitly via its class-scoped symbol table. Fix: in buildFileCallEdges, after resolveCallTargets returns no targets for a this/super receiver call, extract the class name prefix from the caller's qualified name (e.g. Logger.info → Logger) and retry with the fully-qualified method name (Logger._write) in the same file. Fixes the JS benchmark recall (83.3% → 100%) and TS benchmark recall (69.4% → 72.2%), satisfying CI thresholds of 90% and 72% respectively.
…1314) Add inline comments explaining why JS recall is 0.9 and TS recall is 0.72, linking the thresholds to the Phase 8.3e same-class this.method() fix and noting which remaining gaps (interface-dispatch, CHA) are tracked in future phases. Addresses Greptile P2 review comment on the threshold bump.
…roperties / create
Seed composite pts keys from three property descriptor APIs so that
obj.key() can be traced back to the original function reference:
- Object.defineProperty(obj, "key", { value: fn })
→ type_map["obj.key"] = "fn"
- Object.defineProperties(obj, { "k1": { value: v1 }, ... })
→ type_map["obj.k1"] = "v1", …
- const obj = Object.create({ f1, f2 }) (shorthand or pair)
→ type_map["obj.f1"] = "f1", type_map["obj.f2"] = "f2"
Implemented in both the Rust extractor (native engine) and the TS
extractor (wasm engine) for parity. Adds a `define-property.js`
fixture with 5 expected edges (3 defineProperty/defineProperties + 2
create) that all now resolve at 100% recall.
Closes #1320
Greptile SummaryThis PR extends the call-graph resolver to track function references assigned via
Confidence Score: 5/5Safe to merge — the new AST-seeding helpers are well-guarded, the previously flagged string-type and quote-stripping bugs are all fixed, and the Rust/TS implementations are in parity. The change adds three new type-map seeding paths, each with identifier-type and string-type guards that prevent malformed keys. All previously reviewed issues have been corrected. Four Rust unit tests and five new benchmark edges cover the concrete cases, and the full test suite passes without regressions. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AST Walker extractTypeMapWalk] -->|call_expression| B{handleDefinePropertyTypeMap}
A -->|variable_declarator + call_expression| C{Object.create?}
B --> D{method?}
D -->|defineProperty| E[string type guard on key arg]
D -->|defineProperties| F[inner loop over pairs]
E --> G[findDescriptorValue]
F --> G
C -->|proto is object literal| I[seedProtoProperties]
I --> J{child kind?}
J -->|shorthand| K[name equals value]
J -->|pair + identifier| L[key extraction with quote strip]
G --> M[setTypeMapEntry obj.key to fn at 0.85]
K --> M
L --> M
M --> N[Edge builder resolves call site]
Reviews (8): Last reviewed commit: "fix: resolve merge conflicts with main (..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const key = arg1.text.replace(/['"]/g, ''); | ||
| if (!key) return; |
There was a problem hiding this comment.
string type guard on the defineProperty key argument — diverges from the Rust counterpart. The Rust seed_define_property_entries calls extract_string_fragment, which returns None for anything that isn't a "string" AST node. The TS version uses .text.replace(/['"]/g, '') on whatever node is present, so Object.defineProperty(obj, myKey, { value: fn }) produces the entry obj.myKey (treating the variable name literally), and a key like "it's-a-key" would be incorrectly trimmed to its-a-key (the ' stripped by the regex). Both cases create a wrong key that can never match a real call site, polluting the type_map.
| const key = arg1.text.replace(/['"]/g, ''); | |
| if (!key) return; | |
| if (arg1.type !== 'string') return; | |
| const key = arg1.text.replace(/^['"]|['"]$/g, ''); | |
| if (!key) return; |
There was a problem hiding this comment.
Fixed — added if (arg1.type !== 'string') return; guard before key extraction in the defineProperty path, matching the Rust extract_string_fragment behaviour which returns None for non-string AST nodes. Also switched to anchored /^['"]|['"]$/ to avoid stripping quotes within key values.
| const key = keyN.text.replace(/['"]/g, ''); | ||
| const target = findDescriptorValue(valN); |
There was a problem hiding this comment.
defineProperties path for the handleDefinePropertyTypeMap inner loop. The keyN.text.replace(/['"]/g, '') global replace would corrupt keys that contain a single or double quote character (e.g., "O'Brien" → OBrien) and, unlike the Rust seed_descriptor_object, strips quotes globally rather than anchored to the string boundaries. The seedProtoProperties function has the same pattern.
| const key = keyN.text.replace(/['"]/g, ''); | |
| const target = findDescriptorValue(valN); | |
| const key = keyN.type === 'string' ? keyN.text.replace(/^['"]|['"]$/g, '') : keyN.text; | |
| const target = findDescriptorValue(valN); |
There was a problem hiding this comment.
Fixed — updated the defineProperties inner loop to use keyN.type === 'string' ? keyN.text.replace(/^['"]|['"]$/g, '') : keyN.text, matching the Rust seed_descriptor_object approach. The anchored regex prevents corrupting keys that contain apostrophes or quote characters.
| const keyN = child.childForFieldName('key'); | ||
| const valN = child.childForFieldName('value'); | ||
| if (!keyN || !valN || valN.type !== 'identifier') continue; | ||
| const key = keyN.type === 'string' ? keyN.text.replace(/['"]/g, '') : keyN.text; |
There was a problem hiding this comment.
seedProtoProperties for pair keys, so a string key containing an apostrophe or double quote would be trimmed incorrectly.
| const key = keyN.type === 'string' ? keyN.text.replace(/['"]/g, '') : keyN.text; | |
| const key = keyN.type === 'string' ? keyN.text.replace(/^['"]|['"]$/g, '') : keyN.text; |
There was a problem hiding this comment.
Fixed — updated seedProtoProperties to use anchored /^['"]|['"]$/ (with the existing keyN.type === 'string' guard retained). This matches the Rust seed_descriptor_object behaviour and prevents corrupting keys containing internal apostrophes.
…operty key extraction (#1328) - defineProperty path: guard arg1.type !== 'string' before extraction, matching the Rust seed_define_property_entries behaviour (which returns None for non-string AST nodes) - defineProperties path: use anchored /^['"]|['"]$/ to strip only boundary quotes, not quotes within the key value, with string-type guard - seedProtoProperties: same anchored regex fix for pair keys Closes the TS/Rust engine parity gap flagged in Greptile review. (docs check acknowledged)
|
Addressed all 3 Greptile review items (in commit 818a557):
Also resolved the merge conflict with main (commit f8b727b), preserving all additions from both sides:
|
Codegraph Impact Analysis24 functions changed → 25 callers affected across 4 files
|
…ng regression (#1328) On Windows, buildGraph uses closeDbDeferred which schedules the SQLite WAL checkpoint via setImmediate. When afterAll runs before the deferred close executes, fs.rmSync throws EBUSY on graph.db. Call flushDeferredClose before the temp-dir cleanup to ensure the handle is closed synchronously.
|
Fixed Windows CI failure (commit 89f8f70): embedding-regression.test.ts afterAll was calling fs.rmSync on the temp dir before the SQLite handle deferred via closeDbDeferred/setImmediate had time to close, causing EBUSY on Windows. Added flushDeferredClose() before the fs.rmSync call per the documented guidance in src/db/connection.ts. |
Merge origin/main (16a1182, 34988f8) into feat/phase-8.3e: - expected-edges.json: keep define-property edges (pts-define-property, pts-create-prototype) from this branch AND inheritance/prototypes edges from main's #1325; merged file now has 30 total edges - embedding-regression.test.ts: use main's retry-backoff afterAll and TestContext/ctx.skip() pattern; import from src/db/index.js
|
Resolved remaining merge conflicts and lint failure:
|
|
Fixed Windows CI failure (commit eae19de): replaced the manual SharedArrayBuffer/Atomics.wait retry loop in embedding-regression.test.ts afterAll with Node.js's built-in fs.rmSync maxRetries/retryDelay options. The previous fix (89f8f70) used 3 attempts × 100ms = 300ms total, which is not enough for Windows Defender to release file locks. The new approach delegates to Node's native retry mechanism with 10 attempts × 200ms = 2s max wait, which is the idiomatic way to handle EBUSY on Windows. |
- Keep both define-property.js edges (5, from this PR) and bind-call-apply.js edges (3, from main #1330) in expected-edges.json -- total now 33 - Preserve handleDefinePropertyTypeMap, findDescriptorValue, seedProtoProperties, and Object.create composite-key seeding (auto-merged correctly) - Incorporate bind/call/apply tracking code from main into javascript.ts - Update JS edge count comment in resolution-benchmark.test.ts (30 -> 33)
|
Resolved merge conflict with main (commit 38cf57b): PR #1330 (bind/call/apply) added bind-call-apply.js with 3 expected edges and its bind-tracking code to javascript.ts. The merge preserves both sets of changes — all 5 define-property edges (this PR) and the 3 bind-call-apply edges (from main). Updated the edge count comment to 33 in resolution-benchmark.test.ts. |
1 participant
Summary
Object.defineProperty(obj, "key", { value: fn }),Object.defineProperties(obj, { k: { value: fn } }), andconst obj = Object.create({ f1, f2 }), so thatobj.key()call sites can be traced back to the original function reference via the existing composite-key lookup in the edge builderdefine-property.jsfixture with 5 expected edges (3pts-define-property+ 2pts-create-prototype), all resolving at 100% recallTest plan
cargo test -p codegraph-core type_map_from_define— 3 new unit tests pass (334 total)npx vitest run tests/benchmarks/resolution/resolution-benchmark.test.ts— 171 tests pass, JS recall 100% (23/23 edges)npx vitest run --exclude "tests/benchmarks/**"— 2640 tests pass, 11 skipped (no regressions)Closes #1320